-
Notifications
You must be signed in to change notification settings - Fork 800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Javascript and TypeScript build for jetpack-wpcom-mu #34158
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
First pass at migrating an ETK feature. Will need to be rebased once we have script building available.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
5d9de77
to
5135060
Compare
…m/Automattic/jetpack into add/js-build-for-jetpack-wpcom-mu
…m/Automattic/jetpack into add/js-build-for-jetpack-wpcom-mu
I did a functional test following the instructions in #54257 and it looks like it works as expected. The only thing I noticed missing was the assets file that contains dependencies etc |
I'm just done with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did Functional testing
on Simple and Atomic site, and it worked well! 🎉 Thank you so much for your work.
📝
- I also merged
trunk
into this branch. - The assets was served by
<script crossorigin="anonymous" type="text/javascript" src="https://s0.wp.com/_static/??-eJx9T0kOwjAM/BDGLUIgDoi3uIlVUppFjkN5PhFFCIrgZs/iGeOUwMSgHBTJehcgjaV3IaNn6wj4lsYoLJCVNK+HvMI3hy8v+WzuSNBTVpY6gQqZyz/TwJqqBCo0JRP9k8BcAl452ChIRaMnVWe+1JjFYFfcaJFFooBwiqIu9Mt9WeHz0zn6URZm5sxj+gHXUyd/bPdN024Pu81muANc1ni8"></script>
in my case.
/** | ||
* Temporary function to feature flag Sentry by segment. | ||
* | ||
* We'll be testing it on production (simple sites) for a while to see if it's feasible to | ||
* activate it for all sites and perhaps get rid of our custom solution. If it works well, | ||
* we'll activate for all simple sites and look into activating it for WoA, too. | ||
* | ||
* @param int $user_id The user id. | ||
* @return bool | ||
*/ | ||
function wpcom_user_in_sentry_test_segment( $user_id ) { | ||
$current_segment = 10; // Segment of existing users that will get this feature in %. | ||
$user_segment = $user_id % 100; | ||
|
||
/* | ||
* We get the last two digits of the user id and that will be used to decide in what | ||
* segment the user is. i.e if current_segment is 10, then only ids that end in < 10 | ||
* will be considered part of the segment. | ||
*/ | ||
return $user_segment < $current_segment; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in understanding that this feature is disabled because user_id on Atomic is not linked to WordPress.com? I'm asking for my learning purposes 🙂 @daledupreez @obenland
PCYsg-Ewq-p2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okmttdhr, I think that @fullofcaffeine is best-placed to comment on why we're only running this on WPCOM Simple and haven't rolled the functionality out more broadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @okmttdhr! Yes, it's not running on AT. The original plan is to run on AT at some point, and we tried - see p4TIVU-9DI-p2#comment-10922 - but there were some problems. At the time, though, we hadn't implemented Sentry yet, and Sentry should work fine on AT, too. The challenge, though is how to treat errors from AT as AT is the wild west :D - sites can run all sorts of third-party code - so we should have a RFC before proceeding, and Sentry atm is not top-priority for WPCOM, AFAIK.
e77f92a
to
519deda
Compare
Hi Dale! We're really excited about moving Verbum into this. Do you have an idea when it will be deployed? |
@alshakero, we're currently blocked on a functional review of the Sentry code by @fullofcaffeine. @fullofcaffeine, can you try to prioritise the review so we can start work on our next migrations that depend on the build code in this PR? |
@daledupreez Hi! I'm checking this out now; sorry for the delay, had a busy week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, and tests well! I also tested logstash capture (when Sentry is disabled) and it works great. The negative scenario for AT also tests well.
Now that this will live in the jetpack-wpcom-mu
, where can I find more info about deploying it on my own? Or will I need to coordinate JP release with the JP folks?
The only missing piece will be to remove the error-reporting-module
from ETK - let me know if you need a hand there. I think we could ship it just after shipping this one.
Thanks!
Thanks for testing and taking a look, @fullofcaffeine!
At present,
That's my plan -- I'll aim to get this PR shipped on Monday and get the removal PR going then as well. |
7dbc2d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Proposed changes:
webpack
and Jetpack's default Webpack configurationsrc/features/error-reporting
, a project we want to migrate from the Editing Toolkit Plugin in https://github.com/Automattic/wp-calypso/tree/05bbf6a425badcdc73cc9ff27c0fc418c018fb2b/apps/editing-toolkitOther information:
Jetpack product discussion
No product changes for now -- we simply want to prepare for pulling more features from the WordPress.com Editing Toolkit plugin so they're available in a single location across WordPress.com Simple and Atomic.
Does this pull request change what data or activity we track or use?
No changes to data or activity tracking.
Testing instructions:
Local testing
pnpm i
pnpm run build-js
src/build/error-reporting/error-reporting.js
, and that you getsrc/build/error-reporting/error-reporting.asset.php
andsrc/build/error-reporting/error-reporting.js.map
producedpnpm run build-production-js
src/build/error-reporting/
now only containserror-reporting.js
(which should be minified) anderror-reporting.asset.php
Functional testing - WordPress.com Simple
bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/js-build-for-jetpack-wpcom-mu
(as documented in this comment) to apply this code to your sandboxs0.wp.com
and your development site are routed via your WordPress.com sandbox0-sandbox.php
file to force file concatenation and :error-reporting-use-sentry
sticker to your site:wp blog-stickers add --blog_id=[YOUR_BLOG_ID] --sticker=error-reporting-use-sentry --who=[YOUR_USERNAME]
/wp-admin/edit.php
for your test site/wp-admin/post.php
(i.e. we want to make sure you're not viewing the post editor viawordpress.com/post/
)sentry.io
wp-content/plugins/editing-toolkit-plugin/prod/error-reporting/index.php
:https://s0.wp.com/wp-content/mu-plugins/jetpack-mu-wpcom-plugin/moon/vendor/automattic/jetpack-mu-wpcom/src/build/error-reporting/error-reporting.js
sentry.io
Functional testing - WPCOM Atomic
add/js-build-for-jetpack-wpcom-mu
in the search field, and then click "Activate" for the version from this branch/wp-admin/edit.php
for the test sitesentry.io
- we don't have this feature enabled for Atomic sites at this time